Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Security Solution] Revert security_solution_common package which is unnecessary #198294

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[Security Solution] Revert security_solution_common package which is unnecessary #198294

wants to merge 12 commits into from
+143 −545

Conversation

logeekal
Copy link
Contributor

Summary

This PR reverts #189633.

Background

PR : #189633 had created a package security_solution_common so that security components can be easily used in Discover plugins.

But because of recent direction change, I have decided to revert that change which mainly moved flyout code to the security_solution_common package.

Most of the changes that you will see will be path changes replacing security_solution_common.

TL;DR

security_solution_common is being removed as the reason it was created does not exists any more.

@logeekal logeekal added backport v8.17.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team labels Oct 30, 2024
@logeekal
Copy link
Contributor Author

/ci

@logeekal logeekal marked this pull request as ready for review October 30, 2024 13:11
@logeekal logeekal requested review from a team as code owners October 30, 2024 13:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

@logeekal
Copy link
Contributor Author

/ci

tomsonpl
tomsonpl approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defend Workflows Code changes LGTM 👍

seanrathier
seanrathier approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@seanrathier seanrathier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

PhilippeOberti
PhilippeOberti reviewed Oct 30, 2024
View reviewed changes
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few very minor comments

.github/CODEOWNERS Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/flyout/document_details/left/components/tour.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/flyout/document_details/right/components/tour.tsx Outdated Show resolved Hide resolved
x-pack/plugins/security_solution/public/flyout/shared/components/expandable_panel.test.tsx Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [b4ed7a1]

History

davismcphee
davismcphee approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-only review, Data Discovery changes LGTM 👍 Just to confirm, are you removing security-solution-common because you're no longer intending to extract the components and instead use discover_shared for IoC?

@logeekal
Copy link
Contributor Author

Code-only review, Data Discovery changes LGTM 👍 Just to confirm, are you removing security-solution-common because you're no longer intending to extract the components and instead use discover_shared for IoC?

Thanks @davismcphee . To answer your question, Yes, exactly. In current state security-solution-common does not make sense as it was partial decomposition so I reverted it in favour of discover_shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanrathier seanrathier seanrathier approved these changes

@tomsonpl tomsonpl tomsonpl approved these changes

@PhilippeOberti PhilippeOberti PhilippeOberti approved these changes

@davismcphee davismcphee davismcphee approved these changes

@JordanSh JordanSh JordanSh approved these changes

@tiansivive tiansivive Awaiting requested review from tiansivive tiansivive is a code owner automatically assigned from elastic/security-entity-analytics

@ashokaditya ashokaditya Awaiting requested review from ashokaditya ashokaditya is a code owner automatically assigned from elastic/security-defend-workflows

@parkiino parkiino Awaiting requested review from parkiino parkiino is a code owner automatically assigned from elastic/security-defend-workflows

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
backport release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.17.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@logeekal @elasticmachine @seanrathier @tomsonpl @PhilippeOberti @davismcphee @JordanSh @kibanamachine